Conversation
Walkthroughコントローラ／モデルの Newspaper.publish を ActiveSupport::Notifications.instrument に置換し、通知リスナ（UnfinishedDataDestroyer / TimesChannelDestroyer）を ActiveSupport::Notifications の5引数コールバック形式に合わせて購読・シグネチャ更新。UnfinishedDataDestroyer は処理後にユーザの career_path を 0 に設定する追加入力あり。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Controller as Controller (Training/Retirement)
participant ASNotif as ActiveSupport::Notifications
participant Unfinished as UnfinishedDataDestroyer
participant Times as TimesChannelDestroyer
participant Other as 既存購読者 (QuestionNotifier 等)
Controller->>ASNotif: instrument('training_completion.create' / 'retirement.create', user: user)
ASNotif-->>Unfinished: call(_name,_started,_finished,_id,payload)
Unfinished->>Unfinished: 指定ユーザの未チェック製品・WIPレポートを削除
Unfinished->>Unfinished: user.update(career_path: 0)
ASNotif-->>Times: call(_name,_started,_finished,_id,payload)
Times->>Times: times channel 関連破棄処理
ASNotif-->>Other: 他の購読者へ通知
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 分
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (5)test/**/*_test.rb📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.rb📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
test/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
test/**/*⚙️ CodeRabbit configuration file
Files:
app/**/*.{rb,js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-09-01T22:31:57.345ZApplied to files:
📚 Learning: 2025-09-01T22:31:57.345ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matuaya
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/controllers/admin/users_controller.rb(1 hunks)app/controllers/retirement_controller.rb(1 hunks)app/controllers/scheduler/daily/auto_retire_controller.rb(1 hunks)app/controllers/training_completion_controller.rb(1 hunks)app/models/times_channel_destroyer.rb(1 hunks)app/models/unfinished_data_destroyer.rb(1 hunks)config/initializers/active_support_notifications.rb(1 hunks)config/initializers/newspaper.rb(0 hunks)test/models/times_channel_destroyer_test.rb(2 hunks)
💤 Files with no reviewable changes (1)
- config/initializers/newspaper.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/models/times_channel_destroyer.rbapp/controllers/retirement_controller.rbapp/controllers/admin/users_controller.rbtest/models/times_channel_destroyer_test.rbapp/controllers/training_completion_controller.rbconfig/initializers/active_support_notifications.rbapp/models/unfinished_data_destroyer.rbapp/controllers/scheduler/daily/auto_retire_controller.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/times_channel_destroyer_test.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Applied to files:
app/controllers/retirement_controller.rbapp/controllers/admin/users_controller.rbapp/controllers/training_completion_controller.rbconfig/initializers/active_support_notifications.rbapp/controllers/scheduler/daily/auto_retire_controller.rb
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Applied to files:
app/controllers/retirement_controller.rbapp/controllers/admin/users_controller.rbtest/models/times_channel_destroyer_test.rbconfig/initializers/active_support_notifications.rbapp/controllers/scheduler/daily/auto_retire_controller.rb
🧬 Code graph analysis (3)
app/models/times_channel_destroyer.rb (1)
app/models/unfinished_data_destroyer.rb (2)
call(3-10)call(4-9)
test/models/times_channel_destroyer_test.rb (2)
app/models/times_channel_destroyer.rb (2)
call(3-16)call(4-15)app/models/unfinished_data_destroyer.rb (2)
call(3-10)call(4-9)
app/models/unfinished_data_destroyer.rb (1)
app/models/times_channel_destroyer.rb (2)
call(3-16)call(4-15)
🔇 Additional comments (7)
app/models/times_channel_destroyer.rb (1)
4-4: LGTM! ActiveSupport::Notificationsのコールバック仕様に正しく適合しています。メソッドシグネチャがActiveSupport::Notificationsの標準的な5引数形式（name, started, finished, id, payload）に更新されています。未使用パラメータに
_プレフィックスが付けられており、コーディング規約に従っています。app/controllers/admin/users_controller.rb (1)
38-38: LGTM! イベント発行がActiveSupport::Notificationsに正しく移行されています。
Newspaper.publishからActiveSupport::Notifications.instrumentへの置き換えが適切に行われています。イベント名もretirement_createからretirement.createに変更され、Rails標準の命名規則に従っています。Based on learningsapp/controllers/training_completion_controller.rb (1)
18-18: LGTM! 研修終了イベントがActiveSupport::Notificationsに正しく移行されています。
training_completion_createイベントがtraining_completion.createに更新され、新しいイベント発行メカニズムに適合しています。ペイロード構造も保持されており、問題ありません。config/initializers/active_support_notifications.rb (1)
59-65: LGTM! サブスクライバー登録が正しく設定されています。
UnfinishedDataDestroyerとTimesChannelDestroyerがretirement.createとtraining_completion.createの両イベントに購読登録されています。これにより、退会時および研修終了時の重要なデータクリーンアップ処理（未チェックProduct・WIP Report削除、Discord分報チャンネル削除など）が確実に実行されます。Based on learningsapp/controllers/scheduler/daily/auto_retire_controller.rb (1)
18-18: LGTM! 自動退会フローでもイベント発行が正しく移行されています。休会後の自動退会処理においても、
retirement.createイベントがActiveSupport::Notificationsで発行されるよう正しく更新されています。これにより、退会のトリガーに関わらずデータクリーンアップ処理が確実に実行されます。Based on learningstest/models/times_channel_destroyer_test.rb (1)
15-15: テストがプロダクションコードの署名変更に正しく対応しています。両方のテストケース（成功パスと失敗パス）で
TimesChannelDestroyer#callの呼び出しが新しい署名(nil, nil, nil, nil, { user: })に更新されています。未使用のActiveSupport::Notificationsパラメータにnilを渡すのは、ユニットテストとして適切です。テストロジックとアサーションは変更されておらず、引き続きDiscordチャンネル削除の成功・失敗両方のシナリオを検証しています。
Also applies to: 29-29
app/controllers/retirement_controller.rb (1)
17-17:retirement.createイベントサブスクライバー登録に問題ありません
config/initializers/active_support_notifications.rbにてretirement.createが UnfinishedDataDestroyer と TimesChannelDestroyer に購読登録されていることを確認しました- 旧イベント名
retirement_createへの参照が残っていないか、rg -n "retirement_create" configなどで config ディレクトリ全体を再検索してください
|
|
||
| class UnfinishedDataDestroyer | ||
| def call(payload) | ||
| def call(_name, _started, _finished, _id, payload) |
There was a problem hiding this comment.
🧩 Analysis chain
ActiveSupport::Notifications購読者の署名に正しく適合しています。
メソッド署名が ActiveSupport::Notifications のコールバック署名 (name, started, finished, id, payload) に更新されています。未使用の4つのパラメータにアンダースコアプレフィックスが付けられており、適切です。メソッド本体のロジック（payload[:user]の抽出とクリーンアップ処理）は変更されていません。
コーディングガイドラインに基づき、モデルメソッドには対応するユニットテストが必要です。以下のスクリプトを実行して、UnfinishedDataDestroyerのテストが存在し、新しい署名に対応していることを確認してください：
🏁 Script executed:
#!/bin/bash
# Description: Verify UnfinishedDataDestroyer tests exist and match new signature
echo "=== Searching for UnfinishedDataDestroyer test file ==="
fd -t f "unfinished_data_destroyer.*test" test/
echo -e "\n=== Checking test calls to UnfinishedDataDestroyer#call ==="
rg -n -A 2 "UnfinishedDataDestroyer.*\.call" test/ --type=rubyLength of output: 116
以下のスクリプトで spec/ ディレクトリ内のテストファイルと UnfinishedDataDestroyer#call 呼び出しを確認してください。
🏁 Script executed:
#!/bin/bash
echo "=== specディレクトリでのテストファイル検索 ==="
fd -t f "unfinished_data_destroyer.*_spec\.rb" spec/
echo -e "\n=== specディレクトリでのUnfinishedDataDestroyer#call呼び出し検索 ==="
rg -n -A 2 "UnfinishedDataDestroyer.*\.call" spec/ --type=rubyLength of output: 224
UnfinishedDataDestroyerに対応するテストが存在しません。
ActiveSupport::Notificationsのコールバック署名変更（call(name, started, finished, id, payload)）を検証するユニットテストが test/ および spec/ 内に見当たりません。新しい署名に対応したテストを追加してください。
🤖 Prompt for AI Agents
In app/models/unfinished_data_destroyer.rb around line 4, there is no unit test
verifying the ActiveSupport::Notifications callback uses the new signature
call(name, started, finished, id, payload); add tests under test/ (Minitest)
and/or spec/ (RSpec) that subscribe or instrument the notification and assert
UnfinishedDataDestroyer#call is invoked with five arguments: name, started (a
Time), finished (a Time), id, and payload (a Hash). In the tests, use
ActiveSupport::Notifications.instrument to emit a sample event, stub or spy the
UnfinishedDataDestroyer instance (or use allow/expect_any_instance_of) to
capture the received args, and assert the arity and values/types match the new
signature.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the test file exists and review its content
echo "=== Checking if test file exists ==="
fd -t f "unfinished_data_destroyer_test.rb" test/
echo -e "\n=== Content of the test file ==="
cat test/models/unfinished_data_destroyer_test.rb
Length of output: 659
@matuaya さん、テストの追加をありがとうございます！確認しました。
test/models/unfinished_data_destroyer_test.rb のテストは適切に実装されています：
✅ 新しい5パラメータ署名を正しく使用
call(nil, nil, nil, nil, payload)で ActiveSupport::Notifications のコールバック署名に対応
✅ すべての処理を検証
- WIP日報の削除
- 未チェック提出物の削除
career_pathのリセット
テストケースが網羅的で、実装の変更を適切に検証しています。
🧬 Code Graph Analysis Results
<possibly_relevant_code_snippets>
app/controllers/reports_controller.rb (lines 24-30)
def unchecked
return redirect_to reports_path unless admin_or_mentor_login?
@reports = Report.list.page(params[:page]).per(PAGER_NUMBER)
@reports = @reports.unchecked.not_wip
render :index
end
app/models/times_channel_destroyer.rb (lines 3-16)
class TimesChannelDestroyer
def call(_name, _started, _finished, _id, payload)
user = payload[:user]
return unless user.discord_profile.times_id
if Discord::Server.delete_text_channel(user.discord_profile.times_id)
user.discord_profile.times_id = nil
user.discord_profile.times_url = nil
user.discord_profile.save!(context: :retirement)
else
Rails.logger.warn "[Discord API] #{user.login_name}の分報チャンネルが削除できませんでした。"
end
end
end
🧠 Learnings used
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: jun-kondo
PR: fjordllc/bootcamp#0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/work_notification_destroyer.rb`において、このクラスは`app/controllers/works_controller.rb`の`destroy`アクションから呼び出され、`before_action :set_my_work`でIDの妥当性チェックが事前に行われるため、payloadのworkオブジェクトは常に有効であることが保証されている。そのためエラーハンドリングは不要。
Learnt from: reckyy
PR: fjordllc/bootcamp#8356
File: app/models/notification.rb:45-47
Timestamp: 2025-07-15T12:58:02.735Z
Learning: fjordllc/bootcampプロジェクトの`app/models/notification.rb`において、`training_completed`通知種別は`TARGETS_TO_KINDS`ハッシュに含めない設計になっている。これは`training_completed`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/models/unfinished_data_destroyer_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
test/models/unfinished_data_destroyer_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/unfinished_data_destroyer_test.rb
🧠 Learnings (1)
📓 Common learnings
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (3)
test/models/unfinished_data_destroyer_test.rb (3)
5-6: LGTM!テストクラス名とテストケース名が適切です。コーディングガイドラインに従って英語で書かれています。
12-12: ActiveSupport::Notifications対応のシグネチャが正しく使用されています。
callメソッドの呼び出しが、ActiveSupport::Notificationsのサブスクライバー形式（name, started, finished, id, payload）に対応した新しいシグネチャで行われています。テスト内で未使用のパラメータにnilを渡すのは適切です。
7-9: フィクスチャの関連性は正しく設定されています
users(:sotugyou)がreport9（wip: trueの WIP日報）およびproduct6（未チェック提出物）に紐づいていることを確認しました。
| assert_not Report.exists?(wip_report.id) | ||
| assert_not Product.exists?(unchecked_product.id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
WIP日報と未チェック提出物の削除が正しく検証されています。
アサーションはコアとなる動作（WIP日報と未チェック提出物の削除）を検証しており、適切です。
ただし、取得した学習内容によると、UnfinishedDataDestroyerはcareer_pathのリセットも行うとされています。このテストではcareer_pathの動作がカバーされていません。必要に応じて、career_pathのリセットも検証することを検討してください。
以下のスクリプトでUnfinishedDataDestroyerの実装を確認し、career_pathのリセット処理が含まれているか確認してください：
🏁 Script executed:
#!/bin/bash
# Description: Check UnfinishedDataDestroyer implementation for career_path reset
# Search for career_path in UnfinishedDataDestroyer
echo "=== UnfinishedDataDestroyer implementation ==="
rg -A 5 -B 5 'career_path' app/models/unfinished_data_destroyer.rb
# Also check the full implementation
echo "=== Full UnfinishedDataDestroyer class ==="
cat app/models/unfinished_data_destroyer.rbLength of output: 596
テストにcareer_pathリセット検証を追加
test/models/unfinished_data_destroyer_test.rb に以下のアサーションを追記し、career_path が 0 にリセットされることを検証してください。
assert_equal 0, user.reload.career_path🤖 Prompt for AI Agents
In test/models/unfinished_data_destroyer_test.rb around lines 13 to 14, add an
assertion to verify that the user's career_path is reset to 0 after destruction:
after the existing assertions (assert_not Report.exists?(wip_report.id) and
assert_not Product.exists?(unchecked_product.id)), reload the user and
assert_equal 0, user.reload.career_path so the test ensures career_path was
reset.
There was a problem hiding this comment.
@matuaya さん、ご対応ありがとうございます!テストにcareer_pathのリセット検証が追加され、UnfinishedDataDestroyerの動作が包括的にカバーされるようになりました。👍
🧠 Learnings used
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: reckyy
PR: fjordllc/bootcamp#8356
File: app/models/notification.rb:45-47
Timestamp: 2025-07-15T12:58:02.735Z
Learning: fjordllc/bootcampプロジェクトの`app/models/notification.rb`において、`training_completed`通知種別は`TARGETS_TO_KINDS`ハッシュに含めない設計になっている。これは`training_completed`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-notifications-for-retirement-create
branch
from
7430023 to
f052caa
Compare
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-notifications-for-retirement-create
branch
from
f052caa to
d4232af
Compare
|
@yokomaru |
|
matuayaさん、お疲れ様です！ |
|
@yokomaru |
yokomaru
left a comment
yokomaru
left a comment
There was a problem hiding this comment.
matuayaさん、お疲れ様です！
レビューに時間がかかってしまい、大変申し訳ありません🙇♀️
1点レビューいたしました＆Descriptionについて1点だけコメントいたします！
- 検証手順に
foreman start -f Procfile.devの実行を追加する- すでに
foreman start -f Procfile.devでアプリを立ち上げた状態から、別ブランチからchore/replace-newspaper-with-activesupport-notifications-for-retirement-createに切り替えただけだとWIPの日報が削除されませんでした👀 - 詳細は追いきれなかったのですが、Rails起動時に
config/initializers/active_support_notifications.rbを元にイベント購読（subscribe）の設定が行われるようなので、ブランチ切り替え後は再度foreman start -f Procfile.devを実行する旨を手順に追記した方が良いかなと思いました💡
- すでに
全体的に非常に丁寧で読みやすく、検証しやすいPRでした〜！
今後自分がPRを作成する際に参考にさせていただきます🙌
お手数をおかけしますが、ご確認よろしくお願いいたします！
| wip_report = reports(:report9) | ||
| unchecked_product = products(:product6) | ||
| payload = { user: } | ||
|
|
||
| UnfinishedDataDestroyer.new.call(nil, nil, nil, nil, payload) | ||
| assert_not Report.exists?(wip_report.id) | ||
| assert_not Product.exists?(unchecked_product.id) |
There was a problem hiding this comment.
UnfinishedDataDestroyerはユーザーの全てのWIPの日報と未チェックの提出物を削除するので、テストに関しても全てのWIPの日報と未チェックの提出物が削除されたことを確認できた方が実装が担保できそうだと思いました！💡
fixtureの追加 or テスト内で日報と提出物を複数create して、UnfinishedDataDestroyerをコールした後、
ユーザーに紐づくWIPの日報と未チェックの提出物が0件になっていることが確認できたら良いのかな？と思いました🫡
(加えて上記以外の日報と提出物は消えないこと、他のユーザーの日報と提出物は残ることまで検証できたらさらに安全かなと思いますが、最低限↑が確認できていれば問題ないかなと思います！)
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-notifications-for-retirement-create
branch
from
d4232af to
798c9f3
Compare
|
@yokomaru
よろしくお願いいたします🙇♀️
全然急ぎではなかったし全く問題ないので大丈夫です |
There was a problem hiding this comment.
お疲れ様です！また少し時間空いてしまいすみません🙇♀️
Description修正やテストケースの追加など丁寧にご対応いただきありがとうございます！🙌
UnfinishedDataDestroyerテストに関してはCodeRabitの指摘にて不足していたところを追加いただいている部分だと思うので、今回のPRの本質的な部分ではないかもしれないのですが、いくつかコメントさせていただきました！(確認のお時間いただく形となってしまう点については申し訳ありません💦)
現在のテストでも動作自体は担保いただいていると思うのと、
自分もテストケースの作成やテスト設計に関して自信があるわけではないので、
違和感を感じた部分や伝わりづらい部分などがありましたら遠慮なくコメントいただければと思います！🙏
また、下記のように言っていただきとても安心しました・・(今まで軽微な修正のレビューのみだったので取り組むのにも時間がかかってしまい、お時間いただいてしまいました💦)
色々お気遣いいただき本当にありがとうございます🙏
全然急ぎではなかったし全く問題ないので大丈夫です
☺️ 👌
どうぞよろしくお願いいたします！
| setup do | ||
| @user = users(:kimura) | ||
| @reports = 3.times.map do |i| | ||
| Report.create!( | ||
| user: @user, | ||
| title: "wipの日報#{i}", | ||
| description: 'テスト日報', | ||
| wip: true, reported_on: Date.current - i | ||
| ) | ||
| end | ||
| @payload = { user: @user } | ||
| end | ||
| test '#call' do | ||
| @user.update(career_path: 1) | ||
| product = products(:product5) | ||
| report = @reports.first | ||
|
|
||
| UnfinishedDataDestroyer.new.call(nil, nil, nil, nil, @payload) | ||
| assert_not Report.exists?(report.id) | ||
| assert_not Product.exists?(product.id) | ||
| assert_equal 'unset', @user.reload.career_path | ||
| end |
There was a problem hiding this comment.
UnfinishedDataDestroyerの主要な責務は次の3点という認識なので、
#callのテストで全件(複数件)削除も含めて網羅されていると、テストを読んだときに処理内容をより直感的に理解できそうだなと思いました 💡
- ユーザーに紐づくWIPの日報を全削除する
- ユーザーに紐づく未チェックの提出物を全削除する
- ステータス（career_path）の更新
現在、exists?で特定の1件分の日報や提出物の存在チェックをしてくださっていると思うのですが、
#callのテストとしては全件削除できていることを確認できた方が上記の動作を明確に担保できるかなと思いました！
There was a problem hiding this comment.
#callのテストで、WIP日報と未チェック提出物の全件削除を確認するように変更しました！
また、uncheckとwipスコープを使用し、assertでexist?を確認するのではなくcountを確認することで全件削除が明確に分かるようにしました🙏
| assert_not Product.where(id: user_unchecked_products).exists? | ||
| assert_not Report.where(id: user_wip_reports).exists? |
There was a problem hiding this comment.
#callの方で全件削除テストを追加いただければ、2つ目のテストでは「他ユーザーのデータが残っていること」だけ確認できればOKそうだと思いましたっ🙆♂️
| product = products(:product2) | ||
| report = reports(:report24) |
There was a problem hiding this comment.
:product2や:report24がどのデータを指しているかが、ぱっと見だと理解しづらいように感じたので、
変数名に情報を追加いただくか、コメントで何のデータがわかると良いのかなと思いました💡
| assert Report.exists?(other_wip_report.id) | ||
| end | ||
|
|
||
| test 'does not delete checked product or unwip report' do |
There was a problem hiding this comment.
あまり英語に自信はないのですが、否定系のun-は形容詞など状態に関するものにつけるイメージがあり、
WIP(Work In Progress)という名詞につける場合はnon-が自然かな？と思いました💡(non-wip report)
There was a problem hiding this comment.
ご指摘の通り違和感ありますね💦
nonに変更しました！
| @payload = { user: @user } | ||
| end | ||
| test '#call' do | ||
| @user.update(career_path: 1) |
There was a problem hiding this comment.
user.update(career_path: 0) は update!にしておくと更新に失敗した場合も気づけて安心かなと思いました💡
| user: @user, | ||
| title: "wipの日報#{i}", | ||
| description: 'テスト日報', | ||
| wip: true, reported_on: Date.current - i |
There was a problem hiding this comment.
Date.current - i で日付の操作ができるのを知らなかったのでとても勉強になりましたっ！
ループでいい感じにテストデータを作成できるのもとても良いと思ったので参考にさせていただきますっ
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-notifications-for-retirement-create
branch
from
798c9f3 to
d346014
Compare
|
@yokomaru
私もテスト作成にあまり自信がなく、そして知識も乏しいので色々ご指摘いただけてとても嬉しいです
全然気にしないでください！いつでもご都合のよいタイミングで大丈夫です |
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-notifications-for-retirement-create
branch
from
d346014 to
5d3ab16
Compare
| class UnfinishedDataDestroyerTest < ActiveSupport::TestCase | ||
| setup do | ||
| @user = users(:kimura) | ||
| @reports = 3.times.map do |i| |
There was a problem hiding this comment.
@reportsは後のコードで使っていないようなので削除して問題ないかなと思いました💡🙌
|
|
||
| UnfinishedDataDestroyer.new.call(nil, nil, nil, nil, @payload) | ||
|
|
||
| assert_equal 0, Product.unchecked.where(user: @user).count |
There was a problem hiding this comment.
WIPの日報の方はsetupで明示的にデータを作成しているためUnfinishedDataDestroyer実行後に0件であるというテストがわかりやすいのですが、
提出物の方はfixturesのデータに依存しているので未チェックの提出物のデータが存在しているのかがテストを見ただけだとパッと分からないのと、
(あまりないと思いますが)何かしらの理由でfixturesの未チェックのデータが削除・変更された場合でもテストが通ってしまう可能性があるなと思いました…！
自分が件数のチェックが良いと提案しておきながら考慮漏れしていて申し訳ありません🙏🙏🙏
上記にあたり、以下のどちらかの対応があるといいかな？と思いました！
- Reportと同様にProductもsetup内で作成する
- UnfinishedDataDestroyer実行前に未チェックの提出物が存在することを確認する(fixturesに未チェックの提出物があることを活かす)
個人的には 1 がいいかなと思っています！
(bootcampアプリの方針がどうかがわからないのですが)特定のテストでのみ更新・削除などで動的に使いたいデータはテスト内で明示的に作成する方が意図が伝わりやすく影響も受けにくいという認識です💡
フィクスチャを最適に管理するには、よくあるテストケースに適用可能なデフォルトデータのみを用いることです。
https://railsguides.jp/v7.0/testing.html#%E3%83%95%E3%82%A3%E3%82%AF%E3%82%B9%E3%83%81%E3%83%A3%E3%81%A8%E3%81%AF%E4%BD%95%E3%81%8B
テストデータを fixture で用意すべきかどうか？
基本的になるべくテスト内で作りたいです
https://bootcamp.fjord.jp/reports/89239#comment_157960
※ こちらは新規作成の文脈ですが、今回のケース（削除の前提データ）にも考え方として近いかなと思いました！
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-notifications-for-retirement-create
branch
from
935ef8e to
3e37413
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/models/unfinished_data_destroyer_test.rb (1)
41-59: fixtureへの依存を減らすとテストがより明確になりますTest 2とTest 3はfixtureに依存していますが、Test 1のようにテスト内でデータを明示的に作成すると、以下のメリットがあります：
- テストコードだけでデータの状態（他ユーザーのWIP日報、チェック済み提出物など）が分かる
- fixtureの変更の影響を受けにくい
- テストの意図がより明確になる
例えば、Test 2では以下のようにデータを作成できます：
test 'does not delete other users wip report and unchecked product' do other_user = users(:hatsuno) other_user_wip_report = Report.create!( user: other_user, title: '他ユーザーのWIP日報', description: 'テスト', wip: true, reported_on: Date.current ) other_practice = Practice.where.not(id: other_user.products.pluck(:practice_id)).first other_user_unchecked_product = Product.create!( user: other_user, practice: other_practice, body: '提出物', wip: false ) UnfinishedDataDestroyer.new.call(nil, nil, nil, nil, @payload) assert Product.exists?(other_user_unchecked_product.id) assert Report.exists?(other_user_wip_report.id) end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/models/unfinished_data_destroyer_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
test/models/unfinished_data_destroyer_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/unfinished_data_destroyer_test.rb
🧠 Learnings (3)
📓 Common learnings
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Applied to files:
test/models/unfinished_data_destroyer_test.rb
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
PR: fjordllc/bootcamp#9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Applied to files:
test/models/unfinished_data_destroyer_test.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (1)
test/models/unfinished_data_destroyer_test.rb (1)
10-39: 主要な責務を網羅した優れたテストです！以下の点が正しく検証されています：
- 対象ユーザーの全WIP日報が削除されること（
Report.wip.where(user:).countが0）- 対象ユーザーの全未チェック提出物が削除されること（
Product.unchecked.where(user:).countが0）- career_pathが'unset'にリセットされること
テスト内でデータを明示的に作成しているため、テストの意図が明確で、fixtureの変更の影響も受けにくい良い実装です。
|
@yokomaru
よろしくお願いいたします🙏 |
| practices = Practice.where.not(id: user.products.pluck(:practice_id)).take(3) | ||
| practices.each do |practice| | ||
| Product.create!( | ||
| user:, | ||
| practice:, | ||
| body: '提出物', | ||
| wip: false | ||
| ) | ||
| end |
There was a problem hiding this comment.
where.not(...).take(3) で既存データとの重複を避けつつテストデータを生成しているのがとても綺麗＆わかりやすくてすごく勉強になりました🙌
今後必要なデータを取得する際に参考にさせていただきます！！
|
|
||
| class UnfinishedDataDestroyerTest < ActiveSupport::TestCase | ||
| setup do | ||
| @payload = { user: users(:kimura) } |
There was a problem hiding this comment.
setupがシンプルで、@payloadが外部から渡ってくるデータというのがわかりやすくて良いなと思いました！🙌
|
@matuaya conflictの修正をお願いします〜。 |
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-notifications-for-retirement-create
branch
from
3e37413 to
a139940
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/models/unfinished_data_destroyer.rb (1)
8-8:updateではなくupdate!の使用を検討してください。退会時の重要なクリーンアップ処理において、
user.update(career_path: 0)はバリデーションエラーが発生しても例外を発生させません。データの整合性を保証するため、user.update!(career_path: 0)を使用することを推奨します。修正案:
- user.update(career_path: 0) + user.update!(career_path: 0)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/controllers/training_completion_controller.rb(1 hunks)app/models/retirement.rb(2 hunks)app/models/times_channel_destroyer.rb(1 hunks)app/models/unfinished_data_destroyer.rb(1 hunks)config/initializers/active_support_notifications.rb(1 hunks)config/initializers/newspaper.rb(0 hunks)test/models/times_channel_destroyer_test.rb(2 hunks)test/models/unfinished_data_destroyer_test.rb(1 hunks)
💤 Files with no reviewable changes (1)
- config/initializers/newspaper.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- test/models/unfinished_data_destroyer_test.rb
- app/controllers/training_completion_controller.rb
- app/models/times_channel_destroyer.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
test/models/times_channel_destroyer_test.rbconfig/initializers/active_support_notifications.rbapp/models/unfinished_data_destroyer.rbapp/models/retirement.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/times_channel_destroyer_test.rb
🧠 Learnings (10)
📓 Common learnings
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 8356
File: app/models/notification.rb:45-47
Timestamp: 2025-07-15T12:58:02.735Z
Learning: fjordllc/bootcampプロジェクトの`app/models/notification.rb`において、`training_completed`通知種別は`TARGETS_TO_KINDS`ハッシュに含めない設計になっている。これは`training_completed`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:52-56
Timestamp: 2025-09-12T21:17:45.295Z
Learning: Rails upgrade PRにおいて、configuration systemの改善提案（shared configuration fallback等）も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 8356
File: db/fixtures/users.yml:823-826
Timestamp: 2025-07-15T12:33:04.885Z
Learning: fjordllc/bootcampプロジェクトでは、`trainee: true`は「研修に所属していたユーザー」全体を示すフラグであり、研修中のユーザーと研修を終了したユーザーの両方を含む。そのため、`trainee: true`と`training_completed_at`の併存は意図された仕様であり、排他的な関係ではない。
Learnt from: hirokiej
Repo: fjordllc/bootcamp PR: 8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Applied to files:
test/models/times_channel_destroyer_test.rbconfig/initializers/active_support_notifications.rbapp/models/unfinished_data_destroyer.rbapp/models/retirement.rb
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Applied to files:
test/models/times_channel_destroyer_test.rbconfig/initializers/active_support_notifications.rbapp/models/unfinished_data_destroyer.rbapp/models/retirement.rb
📚 Learning: 2025-07-15T12:58:02.735Z
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 8356
File: app/models/notification.rb:45-47
Timestamp: 2025-07-15T12:58:02.735Z
Learning: fjordllc/bootcampプロジェクトの`app/models/notification.rb`において、`training_completed`通知種別は`TARGETS_TO_KINDS`ハッシュに含めない設計になっている。これは`training_completed`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Applied to files:
config/initializers/active_support_notifications.rbapp/models/unfinished_data_destroyer.rb
📚 Learning: 2025-07-23T21:11:21.826Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/work_notification_destroyer.rb`において、このクラスは`app/controllers/works_controller.rb`の`destroy`アクションから呼び出され、`before_action :set_my_work`でIDの妥当性チェックが事前に行われるため、payloadのworkオブジェクトは常に有効であることが保証されている。そのためエラーハンドリングは不要。
Applied to files:
app/models/unfinished_data_destroyer.rb
📚 Learning: 2025-07-23T21:11:21.826Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/inquiry_notifier.rb`において、`NOTIFICATION_SENDER_NAME`で指定される管理者ユーザーは必ず存在することがシステムの前提条件として保証されているため、senderがnilになることは想定されておらず、エラーハンドリングは不要である。
Applied to files:
app/models/unfinished_data_destroyer.rb
📚 Learning: 2025-09-12T21:18:00.834Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
app/models/unfinished_data_destroyer.rb
📚 Learning: 2025-10-22T06:04:36.036Z
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.036Z
Learning: ChecksController#createおよび#destroyでは、Checkの作成・削除とActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックする。これによりWebUI表示とDB状態の整合性を保証している。
Applied to files:
app/models/unfinished_data_destroyer.rb
📚 Learning: 2025-09-12T21:17:26.029Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:19-23
Timestamp: 2025-09-12T21:17:26.029Z
Learning: Rails アップグレードPRにおいて、config_for(:secrets) を使った webhook URL 設定の shared セクション未参照問題は、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
app/models/unfinished_data_destroyer.rb
📚 Learning: 2025-09-12T21:16:47.639Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Applied to files:
app/models/unfinished_data_destroyer.rb
🧬 Code graph analysis (2)
test/models/times_channel_destroyer_test.rb (2)
app/models/times_channel_destroyer.rb (2)
call(3-16)call(4-15)app/models/unfinished_data_destroyer.rb (2)
call(3-10)call(4-9)
app/models/unfinished_data_destroyer.rb (1)
app/models/times_channel_destroyer.rb (2)
call(3-16)call(4-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (4)
test/models/times_channel_destroyer_test.rb (1)
15-15: LGTM！ActiveSupport::Notificationsのコールバック署名に正しく対応しています。
TimesChannelDestroyer#callの新しい5引数署名(name, started, finished, id, payload)に合わせて、テストが正しく更新されています。成功時と失敗時の両方のケースが適切にカバーされています。Also applies to: 29-29
app/models/retirement.rb (1)
85-87: LGTM！ActiveSupport::Notificationsへの移行が適切に実装されています。
Newspaper.publishからActiveSupport::Notifications.instrumentへの移行が正しく行われています。イベント名とペイロード構造も適切で、トランザクション外で呼び出されているため副作用の処理も問題ありません。config/initializers/active_support_notifications.rb (1)
64-70: LGTM！購読者の登録が適切に実装されています。
UnfinishedDataDestroyerとTimesChannelDestroyerの購読者登録が正しく実装されています。両方ともretirement.createとtraining_completion.createイベントに購読しており、既存の購読者パターンと一貫性があります。インスタンスの再利用も効率的です。app/models/unfinished_data_destroyer.rb (1)
4-4: ActiveSupport::Notificationsのコールバック署名に正しく対応しています。メソッド署名が
ActiveSupport::Notificationsのコールバック形式(name, started, finished, id, payload)に更新されています。未使用のパラメータに適切にアンダースコアプレフィックスが付けられています。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/models/retirement_test.rb (1)
23-23: メソッド名の変更が適切に反映されています。
publish_to_newspaperからinstrument_create_eventへの変更は、Newspaper gemからActiveSupport::Notificationsへの移行に対応しており、テストの更新として適切です。オプションとして、ActiveSupport::Notifications.instrumentが正しいパラメータで呼び出されることを検証する専用のテストケースの追加も検討できます。例えば：
test 'instrument_create_event publishes retirement.create event with user' do retirement = Retirement.by_self({}, user: @user) assert_called_with( ActiveSupport::Notifications, :instrument, ['retirement.create', { user: @user }] ) do retirement.send(:instrument_create_event) end endただし、イベントリスナ（UnfinishedDataDestroyer、TimesChannelDestroyer）のテストで実際のイベント処理が検証されている場合は、このレベルのテストは不要かもしれません。
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/models/retirement_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
test/models/retirement_test.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/models/retirement_test.rb
🧠 Learnings (2)
📓 Common learnings
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`はUnfinishedDataDestroyer（未チェックProduct・WIP Report削除、career_pathリセット）とTimesChannelDestroyer（Discord分報チャンネル削除）による重要なデータクリーンアップ処理を実行するため、退会のトリガーが何であろうと（admin、hibernation、user）必ず実行する必要がある。
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 8356
File: app/models/notification.rb:45-47
Timestamp: 2025-07-15T12:58:02.735Z
Learning: fjordllc/bootcampプロジェクトの`app/models/notification.rb`において、`training_completed`通知種別は`TARGETS_TO_KINDS`ハッシュに含めない設計になっている。これは`training_completed`が通知一覧のフィルタ対象ではないためで、意図的な設計判断である。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:52-56
Timestamp: 2025-09-12T21:17:45.295Z
Learning: Rails upgrade PRにおいて、configuration systemの改善提案（shared configuration fallback等）も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Learnt from: reckyy
Repo: fjordllc/bootcamp PR: 8356
File: db/fixtures/users.yml:823-826
Timestamp: 2025-07-15T12:33:04.885Z
Learning: fjordllc/bootcampプロジェクトでは、`trainee: true`は「研修に所属していたユーザー」全体を示すフラグであり、研修中のユーザーと研修を終了したユーザーの両方を含む。そのため、`trainee: true`と`training_completed_at`の併存は意図された仕様であり、排他的な関係ではない。
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:19-23
Timestamp: 2025-09-12T21:17:26.029Z
Learning: Rails アップグレードPRにおいて、config_for(:secrets) を使った webhook URL 設定の shared セクション未参照問題は、設定システム全体の変更として別PRで対応すべきである。
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T06:04:36.036Z
Learning: ChecksController#createおよび#destroyでは、Checkの作成・削除とActiveSupport::Notifications.instrumentによるイベント発行(プラクティスのステータス更新)を同一トランザクション内で実行し、いずれかが失敗した場合は両方をロールバックする。これによりWebUI表示とDB状態の整合性を保証している。
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 0
File: :0-0
Timestamp: 2025-07-23T21:11:21.826Z
Learning: fjordllc/bootcampプロジェクトの`app/models/work_notification_destroyer.rb`において、このクラスは`app/controllers/works_controller.rb`の`destroy`アクションから呼び出され、`before_action :set_my_work`でIDの妥当性チェックが事前に行われるため、payloadのworkオブジェクトは常に有効であることが保証されている。そのためエラーハンドリングは不要。
📚 Learning: 2025-09-01T22:31:57.345Z
Learnt from: matuaya
Repo: fjordllc/bootcamp PR: 9108
File: app/models/after_user_retirement.rb:0-0
Timestamp: 2025-09-01T22:31:57.345Z
Learning: fjordllc/bootcampプロジェクトにおいて、`Newspaper.publish(:retirement_create, { user: user })`は通知処理ではなく、UnfinishedDataDestroyerやTimesChannelDestroyer等による重要なデータクリーンアップ処理を実行するため、管理者による退会（triggered_by: 'admin'）の場合でも必ず実行する必要がある。
Applied to files:
test/models/retirement_test.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
|
@komagata |
app/models/retirement.rb
Outdated
|
|
||
| def publish_to_newspaper | ||
| Newspaper.publish(:retirement_create, { user: @user }) | ||
| def instrument_create_event |
There was a problem hiding this comment.
メソッドは基本動詞であるべきで、こちらだとちょっと意味が通らない用に思いました。
There was a problem hiding this comment.
ご指摘ありがとうございます！
メソッド名をtrigger_retirement_eventに変更しました🙇♀️
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-notifications-for-retirement-create
branch
from
d5df3c6 to
5632ec7
Compare
matuaya
force-pushed
the
chore/replace-newspaper-with-activesupport-notifications-for-retirement-create
branch
from
5632ec7 to
e1c9eab
Compare
|
@komagata |
app/models/retirement.rb
Outdated
|
|
||
| def publish_to_newspaper | ||
| Newspaper.publish(:retirement_create, { user: @user }) | ||
| def trigger_retirement_event |
There was a problem hiding this comment.
クラスとメソッドは クラス.メソッドで主語.動詞のような文章になるイメージです。
クラスがRetirementでメソッドがtrigger_retirement_eventだと両方にretirementが入っているので「頭痛が痛い」のような冗長な文章になっている形です。
これらを踏まえて自然な文になるようにメソッド名を考えてみてください。
There was a problem hiding this comment.
アドバイスありがとうございます！
publishにメソッド名を変更しました🙇♀️
|
@komagata |
komagata
deleted the
chore/replace-newspaper-with-activesupport-notifications-for-retirement-create
branch
3 participants
Issue
概要
retirement_createの通知処理をNewspaperからActiveSupport::Notificationsの仕組みに移行しました。
変更確認方法
chore/replace-newspaper-with-activesupport-notifications-for-retirement-createをローカルに取り込むforeman start -f Procfile.devでサーバーを起動するhajimeでログインする。http://localhost:3000/reportsのページに行き、先ほど作成した日報が表示されていることを確認する。komagataでログインし、http://localhost:3000/reportsのページに行き、WIPの日報が削除されていることを確認する。kenshuでログインする。http://localhost:3000/reportsのページに行き、先ほど作成した日報が表示されていることを確認する。komagataでログインし、http://localhost:3000/reportsのページに行き、WIPの日報が削除されていることを確認する。tomomiでログインし、日報を作成しWIPとして保存する。komagataでログインし、http://localhost:3000/reportsのページに行き、先ほど作成した日報が表示されていることを確認する。tomomiのユーザーページに移動する（http://localhost:3000/users/350839102）。http://localhost:3000/reportsのページに行き、WIPの日報が削除されていることを確認する。Screenshot
UIに変化はないためスクリーンショットは省略しています。
Summary by CodeRabbit
リリースノート
✏️ Tip: You can customize this high-level summary in your review settings.